Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix(pyroscope): switch to in-memory profile handling for AppendIngest #2577

Merged
merged 4 commits into from
Feb 5, 2025
Merged

fix(pyroscope): switch to in-memory profile handling for AppendIngest #2577

merged 4 commits into from
Feb 5, 2025

Conversation

marcsanmi
Copy link
Contributor

@marcsanmi marcsanmi commented Jan 30, 2025 •
edited
Loading

PR Description

Depends on #2574

Changes the profile handling in receive_http and write components related to profiles received and write from /ingest API instead of using pipes for fanout operations.

This matches how Append() already handles profiles (as RawProfile) and simplifies the code by:

  1. Reading the profile body once at the entry point
  2. Creating new readers from the in-memory buffer for each appendable/endpoint
  3. Eliminating complex pipe handling and potential race conditions

The change affects:

  • pyroscope.receive_http component
  • pyroscope.write component

Note: This approach means profiles are held in memory until all appendables finish processing.
This matches how Append() already handles profiles internally, which has proven reliable in production with no memory issues reported so far.

Which issue(s) this PR fixes:
Replaces the previous pipe-based approach that could lead to "read/write on
closed pipe" errors with a more robust in-memory solution.

Notes to the Reviewer

PR Checklist

  • CHANGELOG.md updated
  • Documentation added
  • Tests updated
  • Config converters updated

@marcsanmi marcsanmi requested a review from simonswine January 30, 2025 13:15
simonswine
simonswine approved these changes Jan 31, 2025
View reviewed changes
Copy link
Contributor

@simonswine simonswine left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would love to see a test covering the error condition here with the closed pipe.

I am a bit unsure that we actually need the Pipeset's atomic.bool (I have not tried running code though).

My best guess is as io.MultiWrite will fail on the first error and no longer continue. We would need to move out the Close to happen once all appenadables have read the body.

internal/component/pyroscope/receive_http/receive_http.go Outdated Show resolved Hide resolved
@marcsanmi marcsanmi changed the title fix(pyroscope.receive_http): prevent read/write on closed pipe errors pyroscope: switch to in-memory profile handling for AppendIngest Jan 31, 2025
@marcsanmi marcsanmi force-pushed the marcsanmi/fix-read-write-errors branch from 1a3d3ab to 43e2818 Compare January 31, 2025 15:41
@marcsanmi marcsanmi requested a review from simonswine January 31, 2025 15:42
@marcsanmi
Copy link
Contributor Author

@simonswine I've completely changed the approach after our discussion. Instead of managing pipes and their lifecycle (which required careful coordination of reads/writes/closes), we now:

  1. Read the profile into memory once at the entry point
  2. Create new readers from the in-memory buffer for each appendable
  3. Let each appendable process independently

This matches how Append() already handles profiles internally and eliminates all the complexity around pipe handling. The tests still verify the same functionality (including error cases) but the implementation is much simpler and more reliable.

LMKWYT

@marcsanmi marcsanmi force-pushed the marcsanmi/fix-read-write-errors branch from 43e2818 to 8a4f9bb Compare January 31, 2025 15:46
@marcsanmi marcsanmi changed the base branch from marcsanmi/pyroscope-relabel-component to main January 31, 2025 15:46
@marcsanmi marcsanmi force-pushed the marcsanmi/fix-read-write-errors branch from 8a4f9bb to 6fac4a8 Compare January 31, 2025 15:47
@marcsanmi marcsanmi changed the base branch from main to marcsanmi/pyroscope-relabel-component January 31, 2025 15:47
@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Jan 31, 2025 •
edited
Loading

💻 Deploy preview available: https://deploy-preview-alloy-2577-zb444pucvq-vp.a.run.app/docs/alloy/latest/

@marcsanmi marcsanmi force-pushed the marcsanmi/fix-read-write-errors branch from 6fac4a8 to a0e3479 Compare January 31, 2025 15:52
@marcsanmi marcsanmi changed the base branch from marcsanmi/pyroscope-relabel-component to main January 31, 2025 15:58
@marcsanmi marcsanmi changed the base branch from main to marcsanmi/pyroscope-relabel-component January 31, 2025 15:58
@marcsanmi marcsanmi requested a review from simonswine January 31, 2025 18:21
@marcsanmi marcsanmi marked this pull request as ready for review January 31, 2025 18:21
@marcsanmi marcsanmi requested review from a team as code owners January 31, 2025 18:21
@marcsanmi marcsanmi force-pushed the marcsanmi/pyroscope-relabel-component branch from aea9afe to 6d37eb8 Compare January 31, 2025 23:12
@marcsanmi marcsanmi force-pushed the marcsanmi/fix-read-write-errors branch from 3670c7f to bbf9c76 Compare January 31, 2025 23:15
simonswine
simonswine approved these changes Feb 3, 2025
View reviewed changes
Copy link
Contributor

@simonswine simonswine left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Thanks for your work on that, this makes the whole thing a bit simpler.

mattdurham
mattdurham approved these changes Feb 3, 2025
View reviewed changes
Copy link
Collaborator

@mattdurham mattdurham left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM and cleaner.

@mattdurham
Copy link
Collaborator

Clayton shouldnt need to look at this since it only holds changelog.md

@mattdurham
Copy link
Collaborator

Kicking off the drone build. which is concerning it doesnt block the merge.

@mattdurham
Copy link
Collaborator

Do you mean to merge to marcsanmi/pyroscope-relabel-component? Is this a WIP?

@marcsanmi
Copy link
Contributor Author

Do you mean to merge to marcsanmi/pyroscope-relabel-component? Is this a WIP?

Yes, I had in mind merging it once this PR is approved and merged. I made it from that branch since I originally spotted the issue after introducing the relabel component, thus to fully test the fix I needed the new component. If it's a problem, I can create a new branch and cherry pick the commits.

@marcsanmi marcsanmi force-pushed the marcsanmi/fix-read-write-errors branch 2 times, most recently from bbf9c76 to afdef0e Compare February 4, 2025 16:58
@marcsanmi marcsanmi force-pushed the marcsanmi/fix-read-write-errors branch from afdef0e to 277f86a Compare February 5, 2025 08:55
@simonswine simonswine added the area/pyroscope Issues/PRs primarly affecting `pyroscope.` components label Feb 5, 2025
Base automatically changed from marcsanmi/pyroscope-relabel-component to main February 5, 2025 09:37
@marcsanmi marcsanmi force-pushed the marcsanmi/fix-read-write-errors branch 2 times, most recently from e89f475 to c78f23b Compare February 5, 2025 09:54
@marcsanmi marcsanmi force-pushed the marcsanmi/fix-read-write-errors branch from c78f23b to da1d816 Compare February 5, 2025 11:55
@marcsanmi marcsanmi changed the title pyroscope: switch to in-memory profile handling for AppendIngest fix(pyroscope): switch to in-memory profile handling for AppendIngest Feb 5, 2025
@marcsanmi marcsanmi merged commit f34e849 into main Feb 5, 2025
30 checks passed
@marcsanmi marcsanmi deleted the marcsanmi/fix-read-write-errors branch February 5, 2025 12:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@simonswine simonswine simonswine approved these changes

@mattdurham mattdurham mattdurham approved these changes

@clayton-cornell clayton-cornell Awaiting requested review from clayton-cornell

Assignees
No one assigned
Labels
area/pyroscope Issues/PRs primarly affecting `pyroscope.` components
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@marcsanmi @mattdurham @simonswine